KAFKA-12170: Fix for Connect Cast SMT to correctly transform a Byte array into a string by sknop · Pull Request #9950 · apache/kafka

sknop · 2021-01-22T13:02:16Z

Cast SMT transformation for bytes -> string.
Without this fix, the conversion becomes

ByteBuffer.toString(), which always gives one of these useless results:
"java.nio.HeapByteBuffer[pos=0 lim=4 cap=4]"
"[B@5ec0a365" (for byte array)

With this change, the byte array is converted into a hex string of the
byte buffer content, for example

"FEDCBA9876543210"

Completed with test case and successfully tried out in a
real database conversion.

Committer Checklist (excluded from commit message)

[X ] Verify design and implementation
[ X] Verify test coverage and CI build status
Verify documentation (including upgrade notes)

Cast SMT transformation for bytes -> string. Without this fix, the conversion becomes ByteBuffer.toString(), which always gives this useless result: "java.nio.HeapByteBuffer[pos=0 lim=4 cap=4]" With this change, the byte array is converted into a hex string of the byte buffer content, for example "FEDCBA9876543210" Completed with test case and successfully tried out in a real database conversion.

It turns out that not all connectors produce a HeapBuffer to store bytes. SAP Hana, specifically, just stores a simple byte array that was not captured by the cast transformer. Completed with test case. Fix for https://issues.apache.org/jira/browse/KAFKA-12170

g1geordie · 2021-01-25T06:45:03Z

@sknop hello , It seems like there are some style error .
You can use ./gradlew checkstyleMain checkstyleTest to cehck .

sknop · 2021-01-25T16:30:51Z

@sknop hello , It seems like there are some style error .
You can use ./gradlew checkstyleMain checkstyleTest to cehck .

Thank you @g1geordie for your feedback. I have adjusted the code and it now passes the style check.

mimaison

Thanks for the PR!
It looks good overall, I just left a few minor comments

sknop · 2021-02-18T20:35:39Z

Hi Mickael Thank you so much for your code review. I have adjusted the code according to your suggestions, they all make sense to me. Cheers Sven Erik seknop@gmail.com

…

On 18 Feb 2021, 16:40 +0000, apache/kafka ***@***.***>, wrote: @mimaison

kkonstantine

Thanks for the PR @sknop
Left a few comments inline.

kkonstantine · 2021-02-19T10:07:39Z

+    private static String castByteArrayToString(byte[] array) {
+        StringBuilder sbuf = new StringBuilder();
+        for (byte b : array) {
+            sbuf.append(String.format("%02X", b));


Also, I'm a bit curious. Why are we selecting hex here and not base64 for example?
Hex of course is less efficient.
https://issues.apache.org/jira/browse/KAFKA-12170 does not mention anything about the rational, so I thought I'd ask.

@rhauch wdyt about the representation here?

The reason I suggested Hex representation was that in the use case I encountered - database PK field encoded as BINARY retrieved via CDC Source Connector - the representation in the SQL CLI was Hex, so choosing the same format made it easy to compare source and target.

I would suggest that base64 encoding would validate its own dedicated transformer, with an ability to choose padding, for example

As noted in my previous comment, I agree with @kkonstantine that base64 would be preferable. Doing that would align better with the existing Values class used in Connect's header converter mechanism.

If we want to add support for multiple encodings, we would need to have a KIP since it would likely mean changing the SMT configuration.

If base64 is the better choice we shall go with that. Would we need to adjust the JIRA ticket to reflect this?

I just ran a little test in the JShell:

jshell> byte[] byteArray = new byte[] {(byte) 0xFE, (byte) 0xDC, (byte) 0xBA, (byte) 0x98, 0x76, 0x54, 0x32, 0x10};
byteArray ==> byte[8] { -2, -36, -70, -104, 118, 84, 50, 16 }

jshell> Base64.getEncoder().encode(byteArray)
$2 ==> byte[12] { 47, 116, 121, 54, 109, 72, 90, 85, 77, 104, 65, 61 }

jshell> Base64.getEncoder().encodeToString(byteArray)
$3 ==> "/ty6mHZUMhA="

jshell> Base64.getEncoder().withoutPadding().encodeToString(byteArray)
$4 ==> "/ty6mHZUMhA"

Would you suggest using padding or not?

Connect's Values code just uses Base64.getEncoder().encodeToString((byte[]) value) and does not disable padding. Seems like we should follow the same pattern here. Thoughts, @kkonstantine ?

Yes, I agree. encodeToString is the method I had in mind. Used in Values and JsonConverter for a conversion of bytes to string. It's also more efficient.

…rray. If the ByteBuffer does not have a backing array, we use get(byte[]) to extract the content.

rhauch · 2021-02-22T19:57:31Z

Before this fix, the changes made in #4820 (KAFKA-6684) resulted in toString() being called on byte[] and ByteBuffer. As highlighted in this PR and issue that ByteBuffer.toString() is not useful, but the toString() on byte[] still works. This PR seems to change that behavior, which would not be backward compatible.

The discussion on PR #4820 also talked about making this compatible with Values.convertToString(...), which for byte[] and ByteBuffer results in a base 64 encoded string (with ISO-8859-1 encoding). See the Values code for details.

~~Because of this, it seems much more sensible to also use base64 here for ByteBuffer so it matches the existing behavior with byte[].~~

Actually, the existing code simply outputs the toString() result of a byte[] object, which is of the form [B@22ef9844, which is the Object.toString() implementation that includes the class alias (e.g., [B is a byte array) and the hex representation of the object's hashCode().

However, given that none of the cast forms of byte[] or ByteBuffer[] are useful, then perhaps it's worth adding the previous discussion on #4820 mentioning that:

As @ewencp suggested, for consistency perhaps we can use the same string formats as SimpleHeaderConverter: https://github.com/apache/kafka/blob/trunk/connect/api/src/main/java/org/apache/kafka/connect/data/Values.java,

As mentioned above, the Values.convertToString(...) uses a base 64 encoding. And this aligns with @kkonstantine's suggestion:

Why are we selecting hex here and not base64 for example? Hex of course is less efficient.

I agree that it maybe doesn't suffice in all user situations, so if we also want to support other encodings we'd need other config changes that will require using the KIP mechanism to propose such enhancements.

WDYT?

rhauch · 2021-02-22T20:55:59Z

+    private static String castByteArrayToString(byte[] array) {
+        StringBuilder sbuf = new StringBuilder();
+        for (byte b : array) {
+            sbuf.append(String.format("%02X", b));


As noted in my previous comment, I agree with @kkonstantine that base64 would be preferable. Doing that would align better with the existing Values class used in Connect's header converter mechanism.

If we want to add support for multiple encodings, we would need to have a KIP since it would likely mean changing the SMT configuration.

Change made after code review.

kkonstantine

Thanks for the detailed comments @rhauch
@sknop I agree with @rhauch that base64 would be preferable and consistent with our previous choices. I also agree to punt support of multiple encodings for now.

kkonstantine · 2021-02-24T04:54:47Z

+    private static String castByteArrayToString(byte[] array) {
+        StringBuilder sbuf = new StringBuilder();
+        for (byte b : array) {
+            sbuf.append(String.format("%02X", b));


Yes, I agree. encodeToString is the method I had in mind. Used in Values and JsonConverter for a conversion of bytes to string. It's also more efficient.

This matches the internal format used in other parts of the Connect code.

rhauch

One import-related fix, but otherwise looks good.

Change requested by code review.

sknop · 2021-02-26T09:51:26Z

What about documentation?

Where is the documentation stored and what is the process to change it? It might be useful to point out that the Cast SMT will create base64 strings for byte arrays.

rhauch · 2021-02-26T21:47:49Z

The documentation is auto-generated from the transforms code, via TransformationDoc. Since this is an existing SMT, there are no changes that that TransformationDoc class, but we still have to change the Cast class here and here to mention that byte arrays can be cast to strings.

sknop · 2021-02-27T14:59:22Z

Thank you @rhauch. Just to make sure I understand this correctly - the types mentioned in the cast string are the target casts, not the source tasks. The current code does not allow casting to a byte array, only casting from a byte array.
As I understand it, therefore no documentation changes are required (except maybe for release notes)?

rhauch · 2021-03-02T18:03:02Z

If we don't clarify the documentation, then I think users will be very confused. Can you take a stab at improving/expanding the documentation a bit to clarify the input and output types?

sknop · 2021-03-02T19:36:47Z

Yes of course, totally agree.

@rhauch

Thank you @rhauch for your feedback.

rhauch

Thanks, @sknop! Looks great.

@mimaison, you have a "request changes", so can you please review? Thanks!

mimaison

Thanks for the PR. LGTM

kkonstantine

This LGTM
Thanks @sknop

mimaison · 2021-03-03T22:14:51Z

Test failures are not related:

JDK 8 / org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest.testOneWayReplicationWithAutoOffsetSync()
JDK 11 / org.apache.kafka.connect.integration.RebalanceSourceConnectorsIntegrationTest.testMultipleWorkersRejoining

Merging to trunk

sknop added 2 commits January 10, 2021 17:52

kkonstantine added the connect label Jan 25, 2021

Fixes after failed style check

4213125

mimaison requested changes Feb 18, 2021

View reviewed changes

Changes made after code review by Mickael Maison (@mimaison)

4e74475

kkonstantine reviewed Feb 19, 2021

View reviewed changes

Address code review feedback: protect case where ByteBuffer has not a…

3028511

…rray. If the ByteBuffer does not have a backing array, we use get(byte[]) to extract the content.

rhauch requested changes Feb 22, 2021

View reviewed changes

Kafka has ready-made Utils class for extracting bytes from a ByteBuffer.

411ceb3

Change made after code review.

kkonstantine reviewed Feb 24, 2021

View reviewed changes

Changed conversion to a Base64-encoded string instead of Hex

99d26f4

This matches the internal format used in other parts of the Connect code.

rhauch reviewed Feb 25, 2021

View reviewed changes

Comment thread connect/transforms/src/main/java/org/apache/kafka/connect/transforms/Cast.java Outdated

rhauch self-requested a review February 25, 2021 16:38

Replaced wildcard with single class imports.

e772b37

Change requested by code review.

Updated Overview and Config after code review

ff3239c

Thank you @rhauch for your feedback.

rhauch approved these changes Mar 3, 2021

View reviewed changes

mimaison approved these changes Mar 3, 2021

View reviewed changes

kkonstantine approved these changes Mar 3, 2021

View reviewed changes

mimaison merged commit f06a47a into apache:trunk Mar 3, 2021

Uh oh!

Conversation

sknop commented Jan 22, 2021

Committer Checklist (excluded from commit message)

Uh oh!

g1geordie commented Jan 25, 2021

Uh oh!

sknop commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sknop commented Feb 18, 2021 via email

Uh oh!

kkonstantine left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhauch commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkonstantine left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sknop commented Feb 26, 2021

Uh oh!

rhauch commented Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sknop commented Feb 27, 2021

Uh oh!

rhauch commented Mar 2, 2021

Uh oh!

sknop commented Mar 2, 2021

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

mimaison left a comment

Choose a reason for hiding this comment

Uh oh!

kkonstantine left a comment

Choose a reason for hiding this comment

Uh oh!

mimaison commented Mar 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sknop commented Jan 25, 2021 •

edited

Loading

rhauch commented Feb 22, 2021 •

edited

Loading

rhauch commented Feb 26, 2021 •

edited

Loading